cli/config: error when the config directory is not a directory - #7235
Open
4RH1T3CT0R7 wants to merge 1 commit into
Open
cli/config: error when the config directory is not a directory#72354RH1T3CT0R7 wants to merge 1 commit into
4RH1T3CT0R7 wants to merge 1 commit into
Conversation
Setting DOCKER_CONFIG (or --config) to the config file itself instead of
the directory containing it is a common mistake, and one the CLI handled
inconsistently:
- on Windows, opening "<config.json>/config.json" fails with a "not
exist" error, which load() treats as "no config file present", so the
misconfiguration was silently ignored, and every command ran with
default configuration. "docker logout" then reported success without
writing anything, because there are no credentials to erase in a
defaulted config, and it returns before saving. "docker login" did
fail, but only once it tried to write, with "mkdir <path>\config.json:
The system cannot find the path specified".
- on unix, the same open fails with ENOTDIR, so the CLI did report an
error, but it named a path that cannot exist
("open /tmp/x/config.json/config.json: not a directory").
Stat the config directory when opening the config file fails, and report
the actual problem. The check has to precede the os.IsNotExist branch,
because on Windows this case *is* an "is not exist" error. The write
path is unchanged, and still fails with the mkdir error above.
This adds one stat to the paths where opening the config file already
failed, which includes the common case of a fresh install that has no
~/.docker yet.
Signed-off-by: Artem Lytkin <iprintercanon@gmail.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
Author
|
The two failing checks are the milestone and impact/ label, which I can't set from a fork. Happy to drop the changelog block if you'd rather not have one. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pointing
DOCKER_CONFIG(or--config) at the config file instead of its directory breaks differently on each platform: on unix it fails withopen /tmp/xxx/config.json/config.json: not a directory, naming a path that can't exist, and on Windows it's silently ignored, sodocker logoutreports success without writing anything.When opening the config fails,
load()now stats the config directory and warns if it isn't one. The check has to come before theos.IsNotExistbranch, since on Windows this case is an is-not-exist error, and the underlying error isn't wrapped soerrors.Iscallers can still tell it apart from "no config file present".It warns rather than failing hard: failing hard would mean changing
LoadDefaultConfigFile's signature or moving the check intocli/command/cli.go. Happy to do that in a follow-up if you'd rather.Fixes #5037.